SOLR-16738: Refactor AdminHandlersProxy for better extensibility #3991
Conversation
|
Nice direction. |
There was a problem hiding this comment.
I'm thinking that support for V2 would be easier if the response type, at least, was a type parameter:
AdminHandlersProxy<R> // R = response type: NamedList, or (extension of) SolrJerseyResponse, or InputStreamResponse (i.e.: metrics)
Then, we would have methods like:
processProxiedResponse(String, R)callRemoteNode(String, SolrRequest<R>)- ...
That might leave a bit more implementation details to the classes extendingAdminHandlersProxy, but we would not have to worry about NamedList responses from V2.
Will be used by the proxy's nascent support for v2 APIs.
Code now compiles and tests mostly pass, with the notable exception of the metrics stuff which will definitely require some additional work.
gerlowskija
force-pushed
the
SOLR-16738-refactor-adminhandlersproxy-to-support-v2
branch
from
44d9aab to
ad3aa10
Compare
|
Hey all - I wanted to incorporate some of the advice that @igiguere gave above. Particularly around giving AdminHandlerProxy a type parameter that allows its methods to be a bit more tailored to the particular use-case. I think it worked out really nicely! Slight bad news though - I wanted a fresh start as I was doing this and ultimately ended up using a new branch and force-pushing that here. Which will mangle some of the PR history and conversation-threads. Sorry about that - I should've found a better way. In any case, I'll update the PR description above to better reflect the latest design. And I'm taking this out of "Draft" mode for now and will aim to merge later this week. Tests and check both pass. |
gerlowskija
changed the title
|
I can't think of any end-user impact here that'd merit a changelog. If anyone sees an aspect of this that I'm missing let me know, but otherwise I'm going to add the no-changelog label. |
gerlowskija
deleted the
SOLR-16738-refactor-adminhandlersproxy-to-support-v2
branch
3 participants
https://issues.apache.org/jira/browse/SOLR-16738
Description
AdminHandlersProxy, in its current form is unwieldy to extend to cover our v2 API:
Ultimately, a big part of the problem here is that AdminHandlersProxy is a big static thing that gets used everywhere. As a result it's riddled with special cases to accommodate it's different callers.
Solution
This PR refactors AdminHandlersProxy by making it an abstract class that supports multiple implementations. Each implementation must support the following methods:
boolean shouldProxy()- determine whether a given request needs proxying or notCollection<String> getDestinationNodes()- identify the node(s) that should be proxied to.SolrRequest<?> prepareProxiedRequest()- create a SolrRequest that can be sent to remote nodesvoid processProxiedResponse(String nodeName, NamedList<Object> proxiedResponse)- do something with the response of a proxied request.(The NamedList in the last item above is unfortunate, but note that it's compatible with v2 implementations. See V2RequestProxy for more details.)
Three different AHP implementations are included in this PR to cover both v1 and v2 API requests:
Other notable details include:
org.apache.solr.handler.admin.proxyRemoteRequestProxy.Tests
New unit tests for helper class
WrappedSolrRequestinWrappedSolrRequestTest. Unit tests for new proxy subclasses inGenericV1RequestProxyTestandV2SolrRequestBasedProxyTest.Checklist
Please review the following and check all that apply:
mainbranch../gradlew check.